-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix!: CreateHostedRunnerRequest, UpdateHostedRunnerRequest instead of HostedRunnerRequest
#3973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix!: CreateHostedRunnerRequest, UpdateHostedRunnerRequest instead of HostedRunnerRequest
#3973
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3973 +/- ##
=======================================
Coverage 93.52% 93.53%
=======================================
Files 207 207
Lines 17600 17586 -14
=======================================
- Hits 16461 16449 -12
+ Misses 938 937 -1
+ Partials 201 200 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So HostedRunnerRequest has both image_version and image.version? That seems odd.
In other parts of this repo, we reuse structs and add comments for fields that are used by one endpoint and not another... so are you thinking that splitting this struct into two separate use cases is going to reduce end-user confusion, and therefore is a desirable change?
|
We can't reuse structs in this case. They are different. And yes, splitting this struct into two separate use cases:
Here are comparison with Copilot:
|
UpdateHostedRunnerRequest to UpdateHostedRunnerCreateHostedRunnerRequest, UpdateHostedRunnerRequest instead of HostedRunnerRequest
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @alexandear!
One nit, otherwise LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
Not-Dhananjay-Mishra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thank you, @Not-Dhananjay-Mishra! |



BREAKING CHANGE:
ActionsService.CreateHostedRunnerandEnterpriseService.CreateHostedRunnernow acceptCreateHostedRunnerRequest;ActionsService.UpdateHostedRunnerandEnterpriseService.UpdateHostedRunnernow acceptUpdateHostedRunnerRequest.While working on #3972, I found that
ActionsService.UpdateHostedRunneracceptsHostedRunnerRequestas a body. But it's wrong according to the docs for actions and enterprise.Current:
Should be
image_id(optional) instead of theImage. So, I created a separate structUpdateHostedRunnerRequestwith an optionalimage_idand the rest of the fields (all optional).